Add Notification Registry for ODP Setting Updates #795
Conversation
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/notification_center/notification_registry.ts
Outdated
Show resolved
Hide resolved
| if (this.projectConfigManager.getConfig() != null) { | ||
| this.updateODPSettings(); | ||
| } | ||
| const sdkKey = this.projectConfigManager.getConfig()?.sdkKey; |
There was a problem hiding this comment.
We have sdkKey here (not indirectly from projectConfigManager) in line 132.
We can consider remove "getSdkKey()" new method from projectConfigManager and use a local value.
Also, we have to change "sdkKey" as mandatory (not null), which may be a breaking change in the release.
There was a problem hiding this comment.
Thanks for the catch - swapped out .getConfig()?sdkKey references with local config.sdkKey usage.
Regarding changing sdkKey to be mandatory, this will definitely be a breaking change - especially since there's some nuances (for example, the Lite bundle does not expect sdkKey to begin with). Brainstorming on this one.
packages/optimizely-sdk/lib/modules/datafile-manager/httpPollingDatafileManager.ts
Outdated
Show resolved
Hide resolved
| @@ -175,6 +179,27 @@ export default class Optimizely { | |||
|
|
|||
| this.readyTimeouts = {}; | |||
| this.nextReadyTimeoutId = 0; | |||
|
|
|||
| if (config.odpManager != null) { | |||
There was a problem hiding this comment.
All this logic should come-up after the readyPromise? How config.sdkKey will be non-null if the datafile is still being fetched.
There was a problem hiding this comment.
Moving logic into Promise.all.
Also, as discussed, this should catch for both cases where a new Optimizely instance is provided either an SDK Key or manually includes a datafile.
| if (this.projectConfigManager.getConfig() != null) { | ||
| this.updateODPSettings(); | ||
| } | ||
| if (config.sdkKey != null) { |
There was a problem hiding this comment.
We have 2 sources of sdkKey, here and projectConfigManager. It looks like sdkKey is optional here (when datafile is provided and no polling required). In projectConfigManager can return a valid sdkKey in either case.
So, it may be a better to get sdkKey consistently from projectConfigManager. In this way, we do not see any breaking changes required.
Summary
(Copied from corresponding Java PR: optimizely/java-sdk#501)
notificationCenterto Optimizely andProjectConfigManagerthen the Notification ofUpdateProjectConfigwill not trigger because its handler is getting added in the mainNotificationCenterobject.UpdateProjectConfigwill updateODPConfigevery time irrespective of different NotificationCenter passed by user.Test plan
Issues